Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[schedule] Refactor Schedule, remove React-isms #13582

Merged
merged 2 commits into from
Sep 14, 2018

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Sep 6, 2018

Once the API stabilizes, we will move Schedule into a separate repo. To promote adoption, especially by projects outside the React ecosystem, we'll remove all React-isms from the source and keep it as simple as possible:

  • No build step.
  • No static types.
  • Everything is in a single file.

If we end up needing to support multiple targets, like CommonJS and ESM, we can still avoid a build step by maintaining two copies of the same file, but with different exports.

This commit also refactors the implementation to split out the DOM- specific parts (essentially a requestIdleCallback polyfill). Aside from the architectural benefits, this also makes it possible to write host-agnostic tests. If/when we publish a version of Schedule that targets other environments, like React Native, we can run these same tests across all implementations.

@@ -309,10 +309,10 @@ describe('ScheduleDOM', () => {
const callbackC = jest.fn(() => callbackLog.push('C'));
const callbackD = jest.fn(() => callbackLog.push('D'));

scheduleWork(callbackA); // won't time out
scheduleWork(callbackA, {timeout: 100}); // won't time out
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed to make a small tweak to an existing test: Callbacks are now executed in timeout-order. This is the only semantic change.

@gaearon
Copy link
Collaborator

gaearon commented Sep 6, 2018

Do you expect to fix the timeout issue I encountered as part of this, or is it completely orthogonal?

@pull-bot
Copy link

pull-bot commented Sep 6, 2018

React: size: 🔺+7.2%, gzip: 🔺+5.8%

Details of bundled changes.

Comparing: f6fb03e...3f059be

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js -1.4% -1.3% 83.46 KB 82.27 KB 22.63 KB 22.34 KB UMD_DEV
react.production.min.js 🔺+7.2% 🔺+5.8% 9.54 KB 10.22 KB 3.96 KB 4.18 KB UMD_PROD

schedule

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
schedule.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
schedule.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD
schedule.development.js -8.5% -6.7% 15.34 KB 14.04 KB 4.61 KB 4.3 KB NODE_DEV
schedule.production.min.js 🔺+17.3% 🔺+19.2% 2.78 KB 3.26 KB 1.21 KB 1.45 KB NODE_PROD
Schedule-dev.js -8.6% -7.0% 15.58 KB 14.23 KB 4.66 KB 4.33 KB FB_WWW_DEV
Schedule-prod.js 🔺+7.9% 🔺+12.7% 7.72 KB 8.33 KB 1.91 KB 2.15 KB FB_WWW_PROD

Generated by 🚫 dangerJS

@acdlite
Copy link
Collaborator Author

acdlite commented Sep 6, 2018

@gaearon In a follow-up. Though I might have accidentally fixed it. This PR is just shuffling things around in preparation for bigger changes.

@gaearon
Copy link
Collaborator

gaearon commented Sep 6, 2018

Oh wow. I don't know what exactly changed but the time slicing fixture is now super smooth and doesn't lock up every few seconds. It's actually worrying because I don't like that the experience can change so much without tests also changing.

Interestingly I still don't get into the .didTimeout case you added earlier. But maybe some other change improved scheduling so that we don't actually timeout so often?

@acdlite
Copy link
Collaborator Author

acdlite commented Sep 6, 2018

Yeah we definitely need more tests. I added some in this PR, planning to write many more.

@sebmarkbage
Copy link
Collaborator

I think that we should drop all bundling (rollup), all minification, all build scripts for this package. It should just be a single file in its final location. npm publish is literally just the source of the package. It needs to be super clear exactly what you get (the content of a single file) and super unopinionated.

@acdlite
Copy link
Collaborator Author

acdlite commented Sep 6, 2018

Ok I'll move everything back into one file and just put a big line in between the two halves, like we do with ReactFiberScheduler :D

@sebmarkbage
Copy link
Collaborator

We should probably drop types and put them in separate libdefs. If we need ES module exports maybe we just have two copies of the file and keep them in sync.

@acdlite acdlite changed the title [schedule] Extract DOM-specific logic into separate module [schedule] Refactor Schedule, remove React-isms Sep 7, 2018
@plievone
Copy link
Contributor

plievone commented Sep 8, 2018

I read the tests in this and the continuations pull request briefly through. It seems that most of the tests consume the frame budget exactly, which is nice from the edge case standpoint, but the most common case might be where the last task goes a bit over the frame budget, and it would be good to document this and its variations and implications in the tests.

@TrySound
Copy link
Contributor

TrySound commented Sep 8, 2018

What's the point of not having a build step? Simple babel+commonjs plugin in prepare hook may work fine without effort in supporting two versions.

@gaearon
Copy link
Collaborator

gaearon commented Sep 9, 2018

Hmm. Not sure what's going on here, but if you put

<html>
  <body>
    <script src="../../../build/dist/react.development.js"></script>
    <script src="../../../build/dist/react-dom.development.js"></script>
    <script src="https://unpkg.com/babel-standalone@6/babel.js"></script>
    <div id="container"></div>
    <script type="text/babel">
      ReactDOM.unstable_createRoot(
        document.getElementById('container')
      ).render(<h1>Hello World!</h1>);
    </script>
  </body>
</html>

into packaging/fixtures/babel-standalone/dev.html, and run yarn build core,dom-client,schedule --type=UMD_DEV, you'll see it not commit anything for five seconds until it expires. Seems like a bug? Surely it can complete a single element faster than five seconds.

gaearon
gaearon previously requested changes Sep 9, 2018
Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the case above — is this a bug?

@gaearon
Copy link
Collaborator

gaearon commented Sep 9, 2018

Specifically it seems like in async mode we don't get deeper than renderRoot until we expire.

screen shot 2018-09-09 at 18 35 56

let isHostCallbackScheduled = false;

let timeRemaining;
if (hasNativePerformanceNow) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used before declaration. Hence the bugs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaearon
Copy link
Collaborator

gaearon commented Sep 11, 2018

Can also confirm time slicing fixture still works as intended (incl. the fix to its behavior on timeout) 👍

Once the API stabilizes, we will move Schedule this into a separate
repo. To promote adoption, especially by projects outside the React
ecosystem, we'll remove all React-isms from the source and keep it as
simple as possible:

- No build step.
- No static types.
- Everything is in a single file.

If we end up needing to support multiple targets, like CommonJS and ESM,
we can still avoid a build step by maintaining two copies of the same
file, but with different exports.

This commit also refactors the implementation to split out the DOM-
specific parts (essentially a requestIdleCallback polyfill). Aside from
the architectural benefits, this also makes it possible to write host-
agnostic tests. If/when we publish a version of Schedule that targets
other environments, like React Native, we can run these same tests
across all implementations.
const timeoutId = timeoutIds.get(callback);
timeoutIds.delete(callbackId);
localClearTimeout(timeoutId);
cancelCallback = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why arrow functions? I figured this would just become an ES file eventually?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Erm I don't get it. Why does that matter?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean ES5 file so that it can be published to npm directly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, when you use 'ES file', seems we all think it refer to 'ES6+ file'

Copy link
Collaborator Author

@acdlite acdlite Sep 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I get rid of let/const as well then?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's make it as plain as possible.

flushCallback(firstCallbackNode);
} while (
firstCallbackNode !== null &&
firstCallbackNode.timesOutAt <= getCurrentTime()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since performance.now() calls have a cost, I wonder if we could avoid recalculating it while we have expired work (since if 100 things expired checking the time is unnecessary — they can't "unexpire"). For example by having a nested loop. You read the time in the outer loop, and compare to it in the inner loop. Once you exit the inner loop, you read the time again. If there's nothing new that expired then we exit.

// Keep flushing callbacks until we run out of time in the frame.
while (
firstCallbackNode !== null &&
getTimeRemaining() - getCurrentTime() > 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this could leave a small buffer as a heuristic. So we don't go over by a few ms from last iteration every time.

let node = firstCallbackNode;
do {
if (node.timesOutAt > timesOutAt) {
// This callback is lower priority than the new one.
Copy link
Collaborator

@gaearon gaearon Sep 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to avoid using the word "priority" in this file if we can since mixing that with timeouts changes the meaning of "higher" and "lower".

e.g.

// The new callback times out before this one

That would also map nicely to the sorted order

} while (node !== firstCallbackNode);

if (next === null) {
// No lower priority callback was found, which means the new callback is
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same re:wording

};
getTimeRemaining = () => 0;
} else if (window._sched) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe window.__scheduleMock or something more obscure?

isPerformingWork = false;
if (firstCallbackNode !== null) {
// There's still work remaining. Request another callback.
ensureHostCallbackIsScheduled(firstCallbackNode);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acdlite Seems ensureHostCallbackIsScheduled doesn't use any argument in the function body, can I ask that why we pass the firstCallbackNode here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Flow I think

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm wait this doesn't make sense. I dunno.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I remove this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in the initial commit (7c4ff13) ensureHostCallbackIsScheduled expected a param, but the second commit (5f74f04) changed it to just read from the package-level firstCallbackNode variable since that's always the value that was passed in anyway. Seems like the call sites were just overlooked as part of this change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the lint should warn on this but don't know why not

// to a naive implementation.
var timeoutID = -1;
requestCallback = function(callback, absoluteTimeout) {
timeoutID = setTimeout(callback, 0, true);
Copy link
Contributor

@NE-SmallTown NE-SmallTown Sep 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acdlite The absoluteTimeout argument doesn't been used here, I'm not sure why eslint doesn't warn about this. Should I remove it?

Simek pushed a commit to Simek/react that referenced this pull request Oct 25, 2018
* Refactor Schedule, remove React-isms

Once the API stabilizes, we will move Schedule this into a separate
repo. To promote adoption, especially by projects outside the React
ecosystem, we'll remove all React-isms from the source and keep it as
simple as possible:

- No build step.
- No static types.
- Everything is in a single file.

If we end up needing to support multiple targets, like CommonJS and ESM,
we can still avoid a build step by maintaining two copies of the same
file, but with different exports.

This commit also refactors the implementation to split out the DOM-
specific parts (essentially a requestIdleCallback polyfill). Aside from
the architectural benefits, this also makes it possible to write host-
agnostic tests. If/when we publish a version of Schedule that targets
other environments, like React Native, we can run these same tests
across all implementations.

* Edits in response to Dan's PR feedback
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
* Refactor Schedule, remove React-isms

Once the API stabilizes, we will move Schedule this into a separate
repo. To promote adoption, especially by projects outside the React
ecosystem, we'll remove all React-isms from the source and keep it as
simple as possible:

- No build step.
- No static types.
- Everything is in a single file.

If we end up needing to support multiple targets, like CommonJS and ESM,
we can still avoid a build step by maintaining two copies of the same
file, but with different exports.

This commit also refactors the implementation to split out the DOM-
specific parts (essentially a requestIdleCallback polyfill). Aside from
the architectural benefits, this also makes it possible to write host-
agnostic tests. If/when we publish a version of Schedule that targets
other environments, like React Native, we can run these same tests
across all implementations.

* Edits in response to Dan's PR feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants